-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix] disable imputation on future data #562
Conversation
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
…d unit tests Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: black <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
…taFrame rather than list of index Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
…ter of test_linear_quantile Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea to put it in the missing values transformer, feels like that it was missing a feature indeed.
I added a few comments to handle a few edge cases it thing we might face.
…mine_trailing_null_rows Signed-off-by: lschilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! It looks much cleaner! I think there is one more thing I missed in previous review and we can merge.
For non_null_feature_names to be passable, it should be added here:
https://github.com/OpenSTEF/openstef/blob/main/openstef/model/model_creator.py#L118
Co-authored-by: Egor Dmitriev <[email protected]> Signed-off-by: Lars Schilders <[email protected]>
Signed-off-by: lschilders <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, approved!
In the
MissingValuesTransformer
, data that had missing "future" values, i.e. features with trailingnull
values was imputed with theimputation_strategy
. This is not desirable since it does not make sense to impute data when it is not in between two known values. Implemented a pre-processing step to remove rows with any trailing null values. Filtered the same rows in the labels accordingly.